Config: Make preset config aware of cpu/memory #3636

praveenkumar · 2023-05-08T03:52:29Z

Preset property is bound with cpu/memory/bundle and have default
values for it but our set function doesn't have map around those
settings and a user can do following without any error message.

$ crc config set preset microshift
$ crc config set cpus 2
$ crc config set preset openshift

and when user try to perform crc start then error message shown.

With this PR we are making the set function bit self aware of the
preset and it's bound properties (cpu/memory) and make sure if those
property doesn't validated for the preset then unset those so default
values going to take over.

with this PR following now works as expected.

$ curl -X POST -d '{"properties":{"preset": "microshift"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config
{"Properties":["preset"]}
$ curl -X POST -d '{"properties":{"cpus": "3"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config
{"Properties":["cpus"]}
$ curl -X POST -d '{"properties":{"preset": "openshift"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config
{"Properties":["preset"]}
$ curl -X GET --unix-socket ~/.crc/crc-http.sock http:/c/api/config | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   894  100   894    0     0  56924      0 --:--:-- --:--:-- --:--:-- 59600
{
  "Configs": {
    "bundle": "/Users/prkumar/.crc/cache/crc_vfkit_4.12.13_arm64.crcbundle",
    "consent-telemetry": "no",
    "cpus": 4,
    "disable-update-check": true,
    "disk-size": 31,
    "enable-cluster-monitoring": false,
    "enable-experimental-features": false,
    "enable-shared-dirs": true,
    "host-network-access": false,
    "http-proxy": "",
    "https-proxy": "",
    "ingress-http-port": 80,
    "ingress-https-port": 443,
    "kubeadmin-password": "",
    "memory": 9216,

[...]

anjannath · 2023-05-08T05:16:41Z

pkg/crc/config/config.go

+		if _, err := c.Unset(key); err != nil {
+			return "", err
+		}
+		return "", nil


i think we should print a message letting the user know the config is restored to its default, since we won't see the config option in config view ?

@anjannath Currently also we are not seeing the default configs as part of config view.

right, do you prefer to keep it that way, or show a message saying Configuration property '%s' restored to default value '%s' (since you are in the file ;) )

@anjannath better to keep it that way.

I also think we need to inform the user that we changed their configuration settings. I'll add that.

anjannath · 2023-05-08T05:20:20Z

pkg/crc/config/viper_config_test.go

@@ -121,7 +121,7 @@ func TestViperConfigLoadDefaultValue(t *testing.T) {

 	bin, err := os.ReadFile(configFile)
 	assert.NoError(t, err)
-	assert.JSONEq(t, `{"cpus":4}`, string(bin))
+	assert.JSONEq(t, `{}`, string(bin))


maybe also add a comment here, // default values are not written to config file

gbraad · 2023-05-08T05:29:09Z

Started with:

{
  "consent-telemetry": "yes",
  "cpus": 2,
  "preset": "openshift"
}

PS> crc config set cpus 4

cleared the default:

{
  "consent-telemetry": "yes",
  "preset": "openshift"
}

Note: the default preset remains (?)

PS> crc config set preset microshift

changed the preset:

{
  "consent-telemetry": "yes",
  "preset": "microshift"
}

PS> crc config set cpus 1

Value '1' for configuration property 'cpus' is invalid, reason: requires CPUs >= 2

PS> crc config set cpus 2

does not add the value (as matches default)

PS> crc config set cpus 2

{
  "consent-telemetry": "yes",
  "cpus": 3,
  "preset": "microshift"
}

Now changing to OpenShift again:

PS> crc config set preset openshift

{
  "consent-telemetry": "yes"
}

SO this looks good... though here the default preset was cleared. Earlier it didn't.
Feedback this happened would be appreciated, not high priority... we have #3636.

gbraad · 2023-05-08T05:33:02Z

pkg/crc/config/config.go

+			}
+		}
+		cpu := c.Get(CPUs)
+		if err := validation.ValidateCPUs(cpu.AsInt(), preset); err != nil {


shouldn't we also use disk-size? This is now identical, but would suggest to create an issue to follow-up.

gbraad

I agree to add the 'default values aren't stored' in the test

This PR make sure that if user try to set a default value for a config property then it just unset that property and default value is used. It will also allow not to have default value in the viper config file. Also add `UpdateDefaults` to set and get config handler so API can also make use of it. Without this Patch ``` $ crc config set preset microshift $ crc config set cpus 2 $ crc config set preset openshift $ cat ~/.crc/crc.json { "cpus": 2, "preset": "openshift" } ``` After this patch `~/.crc/crc.json` going to be empty and openshift preset is going to use default `cpus` as `4`.

Preset property is bound with cpu/memory/bundle and have default values for it but our `set` function doesn't have map around those settings and a user can do following without any error message. ``` $ crc config set preset microshift $ crc config set cpus 2 $ crc config set preset openshift ``` and when user try to perform `crc start` then error message shown. With this PR we are making the set function bit self aware of the preset and it's bound properties (cpu/memory) and make sure if those property doesn't validated for the preset then unset those so default values going to take over. with this PR following now works as expected. ``` $ curl -X POST -d '{"properties":{"preset": "microshift"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config {"Properties":["preset"]} $ curl -X POST -d '{"properties":{"cpus": "3"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config {"Properties":["cpus"]} $ curl -X POST -d '{"properties":{"preset": "openshift"}}' --unix-socket ~/.crc/crc-http.sock http:/c/api/config {"Properties":["preset"]} $ curl -X GET --unix-socket ~/.crc/crc-http.sock http:/c/api/config | jq . % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 894 100 894 0 0 56924 0 --:--:-- --:--:-- --:--:-- 59600 { "Configs": { "bundle": "/Users/prkumar/.crc/cache/crc_vfkit_4.12.13_arm64.crcbundle", "consent-telemetry": "no", "cpus": 4, "disable-update-check": true, "disk-size": 31, "enable-cluster-monitoring": false, "enable-experimental-features": false, "enable-shared-dirs": true, "host-network-access": false, "http-proxy": "", "https-proxy": "", "ingress-http-port": 80, "ingress-https-port": 443, "kubeadmin-password": "", "memory": 9216, [...] ```

openshift-ci · 2023-05-08T06:14:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anjannath, gbraad

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [anjannath,gbraad]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-05-08T08:26:38Z

@praveenkumar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`824c752`	link	true	`/test e2e-crc`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gbraad · 2023-05-11T12:46:31Z

Found this reference to some interesting historical fact:
https://github.com/crc-org/crc/pull/2888/files#r770370496

cfergeau · 2023-05-11T15:37:35Z

Will need some follow-up work #3652

openshift-ci bot requested review from evidolob and gbraad May 8, 2023 03:52

praveenkumar force-pushed the issue_3633 branch from cbc8459 to b05b2a0 Compare May 8, 2023 04:52

anjannath reviewed May 8, 2023

View reviewed changes

gbraad reviewed May 8, 2023

View reviewed changes

gbraad approved these changes May 8, 2023

View reviewed changes

openshift-ci bot assigned gbraad May 8, 2023

openshift-ci bot added lgtm approved labels May 8, 2023

praveenkumar added 2 commits May 8, 2023 11:31

praveenkumar force-pushed the issue_3633 branch from b05b2a0 to 824c752 Compare May 8, 2023 06:01

openshift-ci bot removed the lgtm label May 8, 2023

anjannath approved these changes May 8, 2023

View reviewed changes

openshift-ci bot assigned anjannath May 8, 2023

openshift-ci bot added the lgtm label May 8, 2023

anjannath mentioned this pull request May 8, 2023

disk-size should also be preset aware #3638

Open

praveenkumar merged commit 73a4b92 into crc-org:main May 8, 2023

anjannath mentioned this pull request May 8, 2023

[BUG] Switching preset does not check for default values, which can end up being used as a config for other presets #3633

Closed

cfergeau mentioned this pull request May 17, 2023

config: Follow-ups to code resetting cpu/memory after preset change #3665

Merged

anjannath mentioned this pull request Sep 6, 2023

Strange message when setting preset #3825

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config: Make preset config aware of cpu/memory #3636

Config: Make preset config aware of cpu/memory #3636

praveenkumar commented May 8, 2023

anjannath May 8, 2023 •

edited

Loading

praveenkumar May 8, 2023

anjannath May 8, 2023

praveenkumar May 8, 2023

cfergeau May 11, 2023

anjannath May 8, 2023

gbraad May 8, 2023

gbraad commented May 8, 2023

gbraad May 8, 2023

gbraad left a comment

openshift-ci bot commented May 8, 2023

openshift-ci bot commented May 8, 2023

gbraad commented May 11, 2023

cfergeau commented May 11, 2023

Config: Make preset config aware of cpu/memory #3636

Config: Make preset config aware of cpu/memory #3636

Conversation

praveenkumar commented May 8, 2023

anjannath May 8, 2023 • edited Loading

Choose a reason for hiding this comment

praveenkumar May 8, 2023

Choose a reason for hiding this comment

anjannath May 8, 2023

Choose a reason for hiding this comment

praveenkumar May 8, 2023

Choose a reason for hiding this comment

cfergeau May 11, 2023

Choose a reason for hiding this comment

anjannath May 8, 2023

Choose a reason for hiding this comment

gbraad May 8, 2023

Choose a reason for hiding this comment

gbraad commented May 8, 2023

gbraad May 8, 2023

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 8, 2023

openshift-ci bot commented May 8, 2023

gbraad commented May 11, 2023

cfergeau commented May 11, 2023

anjannath May 8, 2023 •

edited

Loading